MCPForUnity/Editor/Helpers/PackageDetector.cs (2)

31-52: Include exception details in log or remove unused variable.

The exception is captured at line 41 but never used. Line 52 only logs the error message string, losing valuable debugging information like stack traces and inner exceptions.

For better debugging of server installation issues, consider one of these approaches:

Option 1 (recommended): Include the exception in the log

-                        if (!string.IsNullOrEmpty(error))
+                        if (capturedEx != null)
                         {
-                            McpLog.Info($"Server check: {error}. Download via Window > MCP For Unity if needed.", always: false);
+                            McpLog.Info($"Server check: {capturedEx}. Download via Window > MCP For Unity if needed.", always: false);
                         }

Option 2: Remove the unused variable if the current logging is intentional

                         string error = null;
-                        System.Exception capturedEx = null;
                         try
                         {
                             // Ensure any UnityEditor API usage inside runs on the main thread
                             ServerInstaller.EnsureServerInstalled();
                         }
                         catch (System.Exception ex)
                         {
                             error = ex.Message;
-                            capturedEx = ex;
                         }

---

57-103: Consider logging suppressed exceptions in catch blocks.

Multiple empty catch blocks throughout the file silently suppress all exceptions. While this may be intentional for "best effort" operations, consider at least logging unexpected exceptions to aid debugging.

For example, at line 57:

-            catch { /* ignore */ }
+            catch (System.Exception ex)
+            {
+                // Log only in development builds or when debugging
+                McpLog.Info($"PackageDetector initialization warning: {ex.Message}", always: false);
+            }

Similar patterns appear at lines 71, 82, 99, and 102.

MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)

43-51: Warn if multiple script matches are found.

The code takes the first GUID without checking if multiple scripts with the same name exist. While unlikely, this could mask installation issues or help with debugging.

Apply this diff to add a warning for multiple matches:

                string[] guids = AssetDatabase.FindAssets($"t:Script {nameof(AssetPathUtility)}");
                
                if (guids.Length == 0)
                {
                    Debug.LogWarning("Could not find AssetPathUtility script in AssetDatabase");
                    return null;
                }
+               
+               if (guids.Length > 1)
+               {
+                   Debug.LogWarning($"Found {guids.Length} instances of AssetPathUtility script. Using first match.");
+               }

                string scriptPath = AssetDatabase.GUIDToAssetPath(guids[0]);

MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs.meta (1)

1-11: Minor formatting inconsistency in empty field syntax.

The empty fields on lines 9-11 (userData, assetBundleName, assetBundleVariant) lack the trailing space after the colon, unlike the other .meta files in this PR where they are formatted as userData: . While Unity typically accepts both formats, maintaining consistency across files improves maintainability.

MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.uss (1)

244-249: Consider using ellipsis for text overflow in path fields.

The text-overflow: clip property (line 248) will abruptly cut off long paths. Using text-overflow: ellipsis would provide a clearer indication that text is truncated, improving user experience when dealing with long file paths.

Apply this diff if you'd like to improve the text truncation behavior:

 .config-path-field > .unity-text-field__input {
     background-color: rgba(0, 0, 0, 0.1);
     font-size: 11px;
     overflow: hidden;
-    text-overflow: clip;
+    text-overflow: ellipsis;
 }

This same consideration applies to other path fields (lines 355, 376) and could be applied consistently.

MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.uxml (1)

11-13: Avoid hard-coding the package version label.

The <ui:Label text="5.0.0" ... /> will drift as soon as we bump the package version. Please bind this value from AssetPathUtility.GetPackageVersion() (already exposed in the window code) or populate it at runtime instead of embedding a literal.

MCPForUnity/Editor/Services/BridgeControlService.cs (2)

38-95: Avoid blocking the Editor thread during verification

ConnectAsync().Wait(…) + blocking stream reads can freeze the UI. Consider running Verify on a background Task and reporting back via callbacks/EditorApplication.delayCall.

---

157-172: Handle CRLF in handshake line

Trim trailing '\r' so FRAMING=1 detection works with CRLF handshakes.

Apply within this range:

-                return Encoding.ASCII.GetString(ms.ToArray());
+                return Encoding.ASCII.GetString(ms.ToArray()).TrimEnd('\r');

MCPForUnity/Editor/Services/ClientConfigurationService.cs (2)

136-142: Guard against null Python dir before auto‑rewrite

If GetPythonServerPath() returns null, auto‑rewrite throws and sets an error. Prefer a clean IncorrectPath status early.

                 string configJson = File.ReadAllText(configPath);
                 string pythonDir = MCPServiceLocator.Paths.GetPythonServerPath();
+                if (string.IsNullOrEmpty(pythonDir))
+                {
+                    client.SetStatus(McpStatus.IncorrectPath);
+                    return client.status != previousStatus;
+                }

---

21-56: Validate UV presence when configuring non‑Claude clients

ConfigureClient currently only checks server.py. For non‑Claude clients that rely on uv, fail fast with a clearer message.

                 string pythonDir = MCPServiceLocator.Paths.GetPythonServerPath();
 
                 if (pythonDir == null || !File.Exists(Path.Combine(pythonDir, "server.py")))
                 {
                     throw new InvalidOperationException("Server not found. Please use manual configuration or set server path in Advanced Settings.");
                 }
+
+                // Most non-Claude clients require uv to run the server
+                if (client.mcpType != McpTypes.ClaudeCode)
+                {
+                    var uvPath = MCPServiceLocator.Paths.GetUvPath();
+                    if (string.IsNullOrEmpty(uvPath))
+                        throw new InvalidOperationException("UV not found. Please install UV or set its path in Advanced Settings.");
+                }

MCPForUnity/Editor/Helpers/ServerInstaller.cs (2)

167-167: Use PlatformNotSupportedException for clarity

Throw a more specific exception type.

-            throw new Exception("Unsupported operating system");
+            throw new PlatformNotSupportedException("Unsupported operating system");

---

733-737: Set a User-Agent header for GitHub downloads (+consider timeouts)

GitHub may 403 requests without a UA. Add a UA header; consider moving to HttpClient with a timeout later.

-                using (var client = new WebClient())
+                using (var client = new WebClient())
                 {
+                    // GitHub requires a User-Agent; improves reliability behind proxies/CDNs
+                    client.Headers.Add("user-agent", $"UnityMCP/{packageVersion} (+https://github.com/CoplayDev/unity-mcp)");
                     client.DownloadFile(downloadUrl, tempZip);
                 }

MCPForUnity/Editor/Helpers/PackageInstaller.cs (1)

39-43: Preserve exception details for diagnostics

Swallowing the exception entirely makes support harder. Log the message at least, while keeping the user-facing info.

-            catch (System.Exception)
+            catch (System.Exception ex)
             {
                 EditorPrefs.SetBool(InstallationFlagKey, true); // Mark as handled
-                McpLog.Info("Server installation pending. Open Window > MCP For Unity to download the server.");
+                McpLog.Info("Server installation pending. Open Window > MCP For Unity to download the server.");
+                McpLog.Warn($"Initial server install attempt failed: {ex.Message}");
             }

MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs (3)

133-141: Guard against missing UXML elements to prevent NREs

If the UXML changes or fails to load an element, later code will NullReferenceException. Add a minimal validation after caching and bail out gracefully.

             // Cache UI elements
             CacheUIElements();
 
             // Initialize UI
-            InitializeUI();
+            // Validate critical elements before proceeding
+            if (new UnityEngine.UIElements.VisualElement[]
+            {
+                debugLogsToggle, validationLevelField, protocolDropdown,
+                connectionToggleButton, clientDropdown
+            }.Any(e => e == null))
+            {
+                Debug.LogError("UI asset mismatch: missing expected elements. Check UXML element names/ids.");
+                return;
+            }
+            InitializeUI();

---

171-178: Throttle per-frame status updates

UpdateConnectionStatus() every Editor frame is unnecessary and can cause churn. Throttle to e.g., 2 Hz.

-        private void OnEditorUpdate()
-        {
-            // Only update UI if it's built
-            if (rootVisualElement == null || rootVisualElement.childCount == 0)
-                return;
-
-            UpdateConnectionStatus();
-        }
+        private double _lastStatusUpdateTime;
+        private void OnEditorUpdate()
+        {
+            if (rootVisualElement == null || rootVisualElement.childCount == 0) return;
+            double now = EditorApplication.timeSinceStartup;
+            if (now - _lastStatusUpdateTime < 0.5) return; // ~2 Hz
+            _lastStatusUpdateTime = now;
+            UpdateConnectionStatus();
+        }

---

726-733: Avoid persistent red label; rely on indicator or reset color

Setting clientStatusLabel to red on error without resetting later can leave the label red permanently. Either remove this or reset in UpdateClientStatus.

             catch (Exception ex)
             {
                 clientStatusLabel.text = "Error";
-                clientStatusLabel.style.color = Color.red;
                 Debug.LogError($"Configuration failed: {ex.Message}");
                 EditorUtility.DisplayDialog("Configuration Failed", ex.Message, "OK");
             }

MCPForUnity/Editor/Services/PathResolverService.cs (2)

146-160: Use portable 'which' invocation (not hardcoded path)

Hardcoding '/usr/bin/which' can fail on some distros. Let the system resolve 'which' via PATH.

-                    var psi = new ProcessStartInfo
-                    {
-                        FileName = "/usr/bin/which",
-                        Arguments = "python3",
-                        UseShellExecute = false,
-                        RedirectStandardOutput = true,
-                        RedirectStandardError = true,
-                        CreateNoWindow = true
-                    };
+                    var psi = new ProcessStartInfo
+                    {
+                        FileName = "which",
+                        Arguments = "python3",
+                        UseShellExecute = false,
+                        RedirectStandardOutput = true,
+                        RedirectStandardError = true,
+                        CreateNoWindow = true
+                    };

---

69-165: Consider clarifying Python detection intent

IsPythonDetected() looks for a Python interpreter, not the MCP server directory. If the intent is to gate features needing the server folder (server.py), consider a separate IsServerInstalled() check or align naming to avoid ambiguity.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

MCPForUnity/Editor/Helpers/AssetPathUtility.cs (1)

85-136: Previous critical issue resolved. Consider caching PackageInfo lookup.

The path conversion logic now properly handles both Package Manager and Asset Store installations, addressing the file I/O issue flagged in the previous review:

• Lines 98-113: For Package Manager installs, resolvedPath provides the absolute file system path (e.g., Library/PackageCache/...).
• Lines 114-120: For Asset Store installs, the virtual Assets/ path is converted to an absolute path via Application.dataPath.

However, PackageInfo.FindForAssembly(typeof(AssetPathUtility).Assembly) is called twice—once in GetMcpPackageRootPath() (line 44) and again here (line 103). Consider caching the PackageInfo result in GetMcpPackageRootPath() and returning it alongside the path, or introducing a helper method that returns the PackageInfo directly to avoid redundant lookups.

Example refactor to eliminate duplicate lookup:

+        private static PackageInfo s_cachedPackageInfo;
+        private static bool s_packageInfoCached;
+
+        private static PackageInfo GetCachedPackageInfo()
+        {
+            if (!s_packageInfoCached)
+            {
+                s_cachedPackageInfo = PackageInfo.FindForAssembly(typeof(AssetPathUtility).Assembly);
+                s_packageInfoCached = true;
+            }
+            return s_cachedPackageInfo;
+        }
+
         public static string GetMcpPackageRootPath()
         {
             try
             {
                 // Try Package Manager first (registry and local installs)
-                var packageInfo = PackageInfo.FindForAssembly(typeof(AssetPathUtility).Assembly);
+                var packageInfo = GetCachedPackageInfo();
                 if (packageInfo != null && !string.IsNullOrEmpty(packageInfo.assetPath))
                 {
                     return packageInfo.assetPath;
                 }
                 
                 // ... rest of method
             }
             // ... catch block
         }
         
         public static JObject GetPackageJson()
         {
             try
             {
                 string packageRoot = GetMcpPackageRootPath();
                 if (string.IsNullOrEmpty(packageRoot))
                 {
                     return null;
                 }

                 string packageJsonPath = Path.Combine(packageRoot, "package.json");

                 // Convert virtual asset path to file system path
                 if (packageRoot.StartsWith("Packages/", StringComparison.OrdinalIgnoreCase))
                 {
                     // Package Manager install - must use PackageInfo.resolvedPath
-                    var packageInfo = PackageInfo.FindForAssembly(typeof(AssetPathUtility).Assembly);
+                    var packageInfo = GetCachedPackageInfo();
                     if (packageInfo != null && !string.IsNullOrEmpty(packageInfo.resolvedPath))
                     {
                         packageJsonPath = Path.Combine(packageInfo.resolvedPath, "package.json");
                     }
                     // ... rest of method
                 }
                 // ... rest of method
             }
             // ... catch block
         }

docs/v6_NEW_UI_CHANGES.md (1)

198-226: Add language identifiers to fenced blocks.

markdownlint (MD040) flags the code fences under “Files Created,” “Helpers,” “UI,” and “CI/CD” because they lack language identifiers. Please tag them (e.g., text or csharp) so the linter passes.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

docs/v6_NEW_UI_CHANGES.md (1)

197-206: Add language identifier to fenced code block.

The fenced code block is missing a language identifier, which prevents proper syntax highlighting and violates Markdown best practices.

Apply this diff:

-```
+```text
 MCPForUnity/Editor/Services/
 ├── IBridgeControlService.cs          # Bridge lifecycle interface

Based on static analysis hints.

MCPForUnity/Editor/Windows/MCPForUnityEditorWindowNew.cs (2)

258-258: Clarify the purpose of the unused server port field.

The serverPortField is initialized with a hard-coded value of "6500" but is never read or used elsewhere in the code. This appears to be reserved for future HTTP support (as indicated by the commented-out HTTPStreaming enum value).

Consider either:

1. Adding a comment explaining this field is reserved for future use, or
2. Removing it until HTTP support is actually implemented.

This will improve code clarity and reduce confusion for future maintainers.

---

466-496: Consider showing actual auto-detected paths instead of placeholder text.

Currently, when no override is set, the UI displays placeholder text like "(auto-detected)" instead of showing the actual path being used. This can make troubleshooting difficult for users who need to verify which paths are active.

Consider updating the logic to display the actual auto-detected path:

 // Python Server Path
 string pythonPath = pathService.GetPythonServerPath();
-if (pathService.HasPythonServerOverride)
-{
-    pythonPathOverride.value = pythonPath ?? "(override set but invalid)";
-}
-else
-{
-    pythonPathOverride.value = pythonPath ?? "(auto-detected)";
-}
+pythonPathOverride.value = pythonPath ?? "(not found)";
+// Optionally add a label indicating if it's an override or auto-detected

 // ... similar for UV Path
 string uvPath = pathService.GetUvPath();
-if (pathService.HasUvPathOverride)
-{
-    uvPathOverride.value = uvPath ?? "(override set but invalid)";
-}
-else
-{
-    uvPathOverride.value = uvPath ?? "(auto-detected)";
-}
+uvPathOverride.value = uvPath ?? "(not found)";

This would give users better visibility into which paths are actually being used by the system.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

MCPForUnity/Editor/Helpers/TelemetryHelper.cs (1)

84-86: Refresh the summary to match the MCP bridge

The XML summary still claims the telemetry logic lives in Python, but this helper now targets the MCP server path. Updating that wording will prevent future confusion when people trace the flow.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro